Tc process refactor #78
Conversation
…t of window ARSNs/IVs with error message
|
Failing tests noted - looking into it. |
Codecov Report
@@ Coverage Diff @@
## dev #78 +/- ##
======================================
Coverage ? 83.30%
======================================
Files ? 18
Lines ? 4116
Branches ? 0
======================================
Hits ? 3429
Misses ? 687
Partials ? 0 Continue to review full report at Codecov.
|
dccutrig
requested review from
jlucas9,
IbraheemYSaleh and
rjbrown2
and removed request for
jlucas9 and
IbraheemYSaleh
| // Now that MAC has been verified, check IV & ARSN if applicable | ||
| if (crypto_config->ignore_anti_replay == TC_IGNORE_ANTI_REPLAY_FALSE) | ||
| { | ||
| status = Crypto_Check_Anti_Replay(sa_ptr, arsn, iv); |
There was a problem hiding this comment.
The anti-replay counter check is not specific to a cryptography interface... This check belongs in crypto_tc.c, not in the libgcrypt/kmc crypto template files.
There was a problem hiding this comment.
Or is there some reason that I'm missing about why this needs to live here?
There was a problem hiding this comment.
This is a good point, looking into it.
There was a problem hiding this comment.
Ok, so I recall now - the reason I did this here is that we cannot validate the ARSN until we know the MAC (if present) is valid - which requires several libgcrypt calls to open/key/validate. Rather than having half of the ARSN checks in crypto_tc.c, and the other half in the decryption crypto_ifs, I opted to place them all the same way. But, I'm open to discussion and more thoughts on this
There was a problem hiding this comment.
Hmmm... I'm still a bit unclear about your reasoning... The cryptography interface process calls (cryptography_if->cryptography_aead_decrypt, cryptography_if->cryptography_validate_authentication) do validate the MAC as part of those functions -- can't Anti-Replay checking just happen after those calls to achieve what you're describing? My primary reasoning for why it belongs in crypto_tc.c instead of the cryptography interface is that this logic will need to be duplicated in both crypto interfaces if it lives at that level, whereas it'll only exist once in crypto_tc.c if it lives there.
There was a problem hiding this comment.
I'm tracking better what you're saying now.... so it's fine if we decrypt the text, and then just discard it if the ARSN/IV turned out be be invalid
There was a problem hiding this comment.
I would think so... for AEAD algorithms, we don't have a choice... for non-AEAD algorithms, we can still have the logic between the authenticate and decrypt calls if we wanted.
…t of window ARSNs/IVs with error message
…vice MAC validation bug
|
Added the MariaDB changes for the acs update... This branch was based off of collab_dev instead of dev, and behind by a number of commits which are conflicting... I did a merge which is now failing. |
|
@dccutrig the ut_tc_process test suite is failing on the EXERCISE_ARSN test because the MAC validation step fails before it gets to the ARSN window validation checks ... This is because of this bug fix which now properly returns after a MAC validation failure: The test data will need to be modified with valid MACs to get past the current part of the process function which is failing and properly test the ARSN windows before this will pass. We should delete the collab_dev branch so that people don't accidentally branch off of that in the future (we should be branching off of dev) |
|
@IbraheemYSaleh Will work on updating the unit tests. I think I mistakenly picked the wrong base branch originally, but if I'm interpreting my log history correctly this was originally based from dev. Agreed we can nuke collab_dev and collab_main if you're ready to do so. |
UTs work out well for ARSN IV, looking for any feedback or comments